Quick suspend support. Enable tx.suspend without xaresource.end #40
Conversation
|
I'm not against adding this new feature in, I'm even considering making it the default behavior. The way BTM implements transaction suspension is a tradeoff between XA spec compliance and pragmatism: the spec mandates that start and end have to be called with TM_SUSPEND and TM_RESUME flags but too many resource drivers do not support this so this alternate version using TM_END / TM_JOIN was put in place instead to notify the resource and potentially enable transaction interleaving. Looking back, it probably was a mistake as almost no resource implements transaction interleaving and this simpler model fits the bill for the more common transaction-per-connection model. Suspend/resume serve no other purpose than enabling the requires_new and not_supported transaction boundaries. While it has other theoretical purposes like enabling nested transactions, BTM does not support that as it's practically useless in the field. I'm rather curious about your 'the current implementation feels weighty' comment. Have you made some performance measurements? Against what DB? If you add some decent test coverage for the feature, you have my blessing for having this feature in. |
|
That's good to hear, i'll add in some test coverage and update the pull-request. Your explanation also answers my other question, about why btm uses TM_END on the suspend call - that's good to know. On my 'weighty' comment; that wasn't based on an observed performance issue, but rather just on the relative cost of communicating with each resource for suspend compared to the 'quick' (local-state-only) approach. |
This is consistent with the suspend behavior of other open source transaction managers. Upgraded unittests to enable configuration updates per testcase.
| } | ||
|
|
||
| public void quickResume() throws XAException { | ||
| if (!this.started) { |
There was a problem hiding this comment.
I've changed my approach on quick suspend/remove slightly to leave the resource-holder in a started state. This seems to be more aligned with the state transitions below (it has not been ended, and start is not expected).
|
Updated with good unittest support. All tests are passing - feels good to go. Let me know what you think. |
| Transaction tx = tm.suspend(); | ||
| assertNull(tm.getTransaction()); | ||
|
|
||
| assertTrue(c.getAutoCommit()); |
There was a problem hiding this comment.
This doesn't make sense. Since the resource did not get notified that it's been detached from the transaction (i.e.: there was no XAResource.end() call) it should not report autocommit as set to true.
I'd go even one step further: if a resource got a XAResource.start() call but BitronixTransactionManager.getCurrentTransaction() returns null, then working with statements should be forbidden or you may end up with work performed on behalf of a suspended transaction.
There was a problem hiding this comment.
I think I see what you are saying, but let me make sure.
The reason that getAutoCommit() is true, is because the call to jdbcPooledConnection.isParticipatingInActiveGlobalTransaction() returns false. That code already reported false when suspended was true (though before my change, that was always associated with end).
As of now, since there is no currentTransaction, this method always returns false. I suppose you are thinking that even though we're suspended, we're effectively in an ActiveGlobalTransaction. If you look at the code that uses the isParticipatingIn.. call that makes sense to me.
So, I might need a bit of help if we were going to try to return true in this state. Since getCurrentTransaction is null - we don't know the exact gtrid to answer with. Should we evaluate all of the xaResourceHolderStates? Is there a better way to determine if we're started, not ended, and suspended?
There is a confusing comment on isParticipatingIn... call. It says "@return true if start() has been successfully called but not end() yet and the transaction is not suspended.". We should update that to reflect the final direction.
|
@lorban i realized that this has been sitting for a bit of time. Do you have any thoughts on my comment above? How do we proceed? |
The current implementation of suspend in btm is to communicate with all resources to end the current transaction. Resume then also communicates to all resources to re-enlist in the suspended transaction.
Other open-source transaction-managers do this in a much simpler way. They simply move aside the current transaction (by nulling the current theadcontext), clearing the way for a new transaction to continue in its place. Resume is then only a matter of restoring the original transaction in the place it was in.
Since suspend is a necessary part of a requires_new transaction boundary efficiency is very important; the current implementation feels weighty.
I'm certain that the current approach was selected to help with more complex usecases than a simple requires_new; could you tell me more about that?
Finally, i don't think this commit is in a state that its ready to incorporate into the main. I would like to add a few unittests that target the functionality - but i just wanted to start a conversation, and i figured that would be easiest with some starter code. Fwiw, this is operating well on my current system (which is quite large).